Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add CvTONNX Config #1131

Merged
merged 3 commits into from
Jun 28, 2023
Merged

Add CvTONNX Config #1131

merged 3 commits into from
Jun 28, 2023

Conversation

rishabbala
Copy link
Contributor

@rishabbala rishabbala commented Jun 23, 2023

What does this PR do?

Added changes for CvT model

Fixes part of #555

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you make sure to update the documentation with your changes?
  • Did you write any new necessary tests?

@michaelbenayoun @fxmarty

@rishabbala rishabbala changed the title CvTOnnx Add CvTONNX Config Jun 23, 2023
@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Jun 26, 2023

The documentation is not available anymore as the PR was closed or merged.

@fxmarty
Copy link
Contributor

fxmarty commented Jun 26, 2023

Hi, thank you for the addition! Tests indeed pass, except the style one. Could you run make style?

@@ -550,6 +550,11 @@ def inputs(self) -> Dict[str, Dict[int, str]]:
return {"pixel_values": {0: "batch_size", 1: "num_channels", 2: "height", 3: "width"}}


class CvTOnnxConfig(ViTOnnxConfig):
DEFAULT_ONNX_OPSET = 13
ATOL_FOR_VALIDATION = 1e-2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems pretty high tbh, do you have any idea which op makes it fail?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you talking about the tolerance or the opset version? Im not sure why lower tolerance doesn't work, but einsum causes an error for lower opset versions

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am talking about the tolerance, we might have a hidden bug if we need 1e-2, or we might not, but just wanted to raise the potential issue here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any way this can be checked?

@rishabbala
Copy link
Contributor Author

@fxmarty Done

Copy link
Contributor

@fxmarty fxmarty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you for the addition!

@rishabbala Maybe before merging, could you try to run the export on a larger CvT model and see what the diff is versus pytorch in the validation?

@rishabbala
Copy link
Contributor Author

The diff for cvt-13 is 3.5e-3, for cvt-13-384-22k it is 6e-3 and for cvt-21 it is 2.7e-3. Should I reduce the tolerance?

@fxmarty
Copy link
Contributor

fxmarty commented Jun 28, 2023

Sounds fine to me, thanks!

@fxmarty fxmarty merged commit 282dd38 into huggingface:main Jun 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants